Add encode implementations for SocketAddr and IpAddr#87
Add encode implementations for SocketAddr and IpAddr#87rnijveld wants to merge 2 commits intoprometheus:masterfrom
Conversation
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
In general I am not opposed to this change. Though I would guess that in most cases, this is not what one wants. In case the number of unique IP addresses is large, this can easily lead to a cardinality explosion.
https://www.robustperception.io/cardinality-is-key/
What do you think of adding a warning doc comment to the implementations to make sure users are aware of what they are doing?
Also would you mind resolving the merge conflicts?
|
Friendly ping @rnijveld. |
rnijveld
force-pushed
the
encode-ip-socket
branch
from
c8d94d6 to
cbd2ba2
Compare
rnijveld
force-pushed
the
encode-ip-socket
branch
from
cbd2ba2 to
f8caf41
Compare
|
@mxinden Sorry about the slow response, life got in the way a little. I definitely agree with not wanting to use this for high cardinality situations. Although of course such cases could happen just as easily with other label types, I do agree that IP addresses might more quickly result in a cardinality explosion when not used correctly. I've added a little warning text to each of the implementations, I hope that resolves your concerns, let me know if there's anything else I can do! |
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
Sorry for the delay here.
Thanks for adding the warning. One confusion on my end.
| /// distinct values is low (i.e. low cardinality). In all other cases you should | ||
| /// combine your metrics into a single metric instead. Especially bad examples |
There was a problem hiding this comment.
In all other cases you should combine your metrics into a single metric instead.
I don't understand this. How is combining multiple metrics into one solving the issue of a cardinality explosion?
| /// distinct values is low (i.e. low cardinality). In all other cases you should | ||
| /// combine your metrics into a single metric instead. Especially bad examples |
|
It is effectively never appropriate to use an address as a label, for reasons of cardinality already discussed. |
|
I don't think "never" is correct. One use-case I can come up with is exposing ones listening address as an info metric. See e.g. kubernetes/kube-state-metrics#498 as an example on how far one can push cardinality in Prometheus with info metrics. |
3 participants
Something I encounter quite often is having metrics based on connection information, so I frequently find myself wanting to use either a
SocketAddrorIpAddrin a label.